Skip to content

Fix 4725 summarize dialog cancel #5108

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
akankshahu wants to merge 6 commits into
base: develop
Choose a base branch
from
Open

Fix 4725 summarize dialog cancel #5108

akankshahu wants to merge 6 commits into from

Conversation

@akankshahu
Copy link
Contributor

@akankshahu akankshahu commented Dec 13, 2025

Fixes #4725

Checklist

  • My pull request has a descriptive title (not a vague title like Update index.md).
  • My pull request targets the develop branch of the repository
  • My commit messages follow best practices.
  • My code follows the established code style of the repository.
  • I added tests for the changes I made (if applicable).
  • I added or updated documentation (if applicable).
  • I tried running the project locally and verified that there are no
    visible errors.

Developer Certificate of Origin

Developer Certificate of Origin 
Developer Certificate of Origin
Version 1.1

Copyright (C) 2004, 2006 The Linux Foundation and its contributors.
1 Letterman Drive
Suite D4700
San Francisco, CA, 94129

Everyone is permitted to copy and distribute verbatim copies of this
license document, but changing it is not allowed.


Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the best
    of my knowledge, is covered under an appropriate open source
    license and I have the right under that license to submit that
    work with modifications, whether created in whole or in part
    by me, under the same open source license (unless I am
    permitted to submit under a different license), as indicated
    in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including all
    personal information I submit with it, including my sign-off) is
    maintained indefinitely and may be redistributed consistent with
    this project or the open source license(s) involved.

@akankshahu akankshahu force-pushed the fix-4725-summarize-dialog-cancel branch from 02a3e56 to 2b73cf5 Compare December 13, 2025 16:17
@akankshahu akankshahu force-pushed the fix-4725-summarize-dialog-cancel branch from 2b73cf5 to 0935c0a Compare December 13, 2025 16:19
@akankshahu
Copy link
Contributor Author

akankshahu commented Dec 16, 2025

@Anish9901 Could you please review it?

@pavish pavish self-assigned this Dec 16, 2025
@pavish pavish self-requested a review December 16, 2025 11:10
@pavish pavish added the pr-status: review A PR awaiting review label Dec 16, 2025
pavish
pavish requested changes Dec 19, 2025
View reviewed changes
Copy link
Member

@pavish pavish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@akankshahu This PR introduces a regression.

When the user clicks on the "No do not summarize" button, the column should be added without summarization. This is no longer working in this PR.

This is because the confirm utility treats both closing the modal and the cancel button in the same way, and this particular use-case repurposes cancel to add the column without summarization.

The right way to do this would be to:

  1. Remove the confirm utlitiy here
  2. Add a separate Modal which looks the same, but both the buttons act as independent actions, one for adding the column without summarization, and the other for adding the column with summarization.
  3. Closing that modal should not add the column.
Image

@pavish pavish added pr-status: revision A PR awaiting follow-up work from its author after review and removed pr-status: review A PR awaiting review labels Dec 19, 2025
@pavish pavish assigned akankshahu and unassigned pavish Dec 19, 2025
@akankshahu
Fix mathesar-foundation#4725: Replace confirm utility with custom modal
89d49d5 
- Created SummarizeColumnModal component with independent action buttons
- Removed confirm utility from InputSidebar
- 'Yes' button: adds column with summarization
- 'No' button: adds column without summarization
- Closing modal: does not add column (fixes regression)
@akankshahu akankshahu force-pushed the fix-4725-summarize-dialog-cancel branch from eaddb2e to 89d49d5 Compare January 14, 2026 05:34
@akankshahu
Copy link
Contributor Author

akankshahu commented Jan 14, 2026

Hello @pavish ,I tried solving for the issue .Could you please review it?

@akankshahu akankshahu requested a review from pavish January 20, 2026 03:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pavish pavish Awaiting requested review from pavish

Requested changes must be addressed to merge this pull request.

Assignees

@akankshahu akankshahu

Labels

pr-status: revision A PR awaiting follow-up work from its author after review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Change behavior for closing the "Summarize" dialog box in Explorations.

2 participants

@akankshahu @pavish